Support Closure device type#2875
Conversation
|
Duplicate profile check: Warning - duplicate profiles detected. |
|
Channel deleted. |
Test Results 72 files 509 suites 0s ⏱️ Results for commit 470bfba. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 470bfba |
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
2 times, most recently
from
611fc50 to
7a0bf8b
Compare
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
3 times, most recently
from
b497ce6 to
3e00e9d
Compare
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
from
542367b to
44c80b1
Compare
|
@nickolas-deboom would it make more sense to implement this closure logic as a subdriver? That way, we would overwrite the capability handlers rather than add a bunch of else's to the handlers |
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
from
44c80b1 to
50c21ea
Compare
I think that might be a good idea |
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
3 times, most recently
from
4b6eba2 to
84e38ff
Compare
| optional: true | ||
| categories: | ||
| - name: Blind | ||
| - id: windowShade4 |
There was a problem hiding this comment.
Why are there multiple components in each of these profiles, and why are there 4 in each? Is 4 an arbitrarily selection?
There was a problem hiding this comment.
Right, 4 is arbitrary since i wasn't sure how many closure panels to typically expect (the spec just says 0+). The example devices I've seen so far have only had up to 2 so I think 4 should be good however I will define this as a constant as you suggested in another comment
There was a problem hiding this comment.
To clarify, a closure consists of 1 endpoint implementing the ClosureControl cluster and then any number of endpoints of the Closure Panel device type that each implement the ClosureDimension cluster. So the number of components in these profiles that are used would reflect the number of ClosureDimension endpoints.
| -- Single ClosureDimension: enable the capability on the main component. | ||
| local found_main = false | ||
| for _, entry in ipairs(optional_caps) do | ||
| if entry[1] == "main" then |
There was a problem hiding this comment.
this might be cleaner to do something like this where you define a main_component_capabilities table earlier in the function and then insert it into the table at the end, that way you can just consistently add to the main capabilities without needing to find them within this optional caps struct. You can just define main_component_capabilities up on line 143 and then insert into it directly for the battery capability checking in line 144-148, and then insert directly in this handling as well.
Then at the end of the function, just push the main_component_capabilities into the optional_caps if it is not null.
| if attr.value == 0x0C then | ||
| match_profile(device, battery_support.BATTERY_PERCENTAGE) | ||
| if is_closure then | ||
| device:set_field(CLOSURE_BATTERY_SUPPORT, battery_support.BATTERY_PERCENTAGE, {persist = true}) |
There was a problem hiding this comment.
Could we just pass the battery support value in the match_profile_for_closure function, like what is done for the match_profile function?
Furthermore, could we have the match_profile function itself handle both closures and other types? You could just add a line at the top of the match_profile function that calls match_profile_for_closure if the device has a ClosureControl ep. It would essentially just be centralizing the logic in this function to the match_profile function itself, which I think would be a little cleaner.
There was a problem hiding this comment.
We need to save it in a field because we need to wait until we have both the battery support and closure tag info before profiling, and the ordering isn't guaranteed. This matches the system used in other drivers where it's required to wait for multiple attribute reports before running match_profile.
Furthermore, could we have the match_profile function itself handle both closures and other types?
Closure support was moved into a subdriver in 541b896, so now it has it's own match_profile function that is specific to closures - is that sufficient in your eyes?
|
@hcarter-775 I made an update to move the closure handling to a subdriver in 541b896. Lemmeknow how that looks |
|
@nickolas-deboom I haven't read the specific closures logic too closely (even when making the initial suggestion), but from the subdriver perspective idea, this looks really great! |
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
4 times, most recently
from
583ba58 to
04f8128
Compare
hcarter-775
left a comment
hcarter-775
left a comment
There was a problem hiding this comment.
With the testing done and the gating we have behind the subdriver implementation, I think this is good to go for merging to main
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
from
04f8128 to
470bfba
Compare
3 participants
Check all that apply
Type of Change
Checklist
Description of Change
Add support for the Closure device type.
Note that these changes were originally in #2751, but that PR was based on a branch that would migrate the matter-window-covering driver to matter-switch, which might be done at some point but is a much larger scope. In the interest of supporting this device type sooner, this PR implements this code in matter-window-covering.
Summary of Completed Tests
Tested with VDA Closure and CHIP example app.